add MemIndexPage::err_ to cache the result error of ReadPage#422
Merged
add MemIndexPage::err_ to cache the result error of ReadPage#422
Conversation
WalkthroughAdds per-page error state to MemIndexPage (new private member and public accessors) and updates IndexPageManager allocation, deallocation, and async load/wait paths to preserve or clear that state depending on pinning; also exposes buffer-pool query helpers (IsFull, GetBufferPoolUsed, GetBufferPoolLimit). Changes
Sequence DiagramsequenceDiagram
participant AsyncLoader as Async Page Loader
participant PageMgr as Index Page Manager
participant Page as MemIndexPage
participant PinTracker as Pin Tracker
rect rgba(200,150,200,0.5)
Note over AsyncLoader,PinTracker: Async read fails
AsyncLoader->>PageMgr: report read error (err)
PageMgr->>PinTracker: is page pinned?
PinTracker-->>PageMgr: pinned / not pinned
alt pinned
PageMgr->>Page: SetError(err)
else not pinned
PageMgr->>PageMgr: FreePage(page)
end
end
rect rgba(150,200,150,0.5)
Note over AsyncLoader,PageMgr: Wait completes with error
AsyncLoader->>PageMgr: wait returns error
PageMgr->>Page: Error()
Page-->>PageMgr: return stored error
PageMgr->>PinTracker: is page pinned?
PinTracker-->>PageMgr: pinned / not pinned
alt not pinned and has error
PageMgr->>PageMgr: FreePage(page)
else pinned
PageMgr->>PageMgr: keep page, propagate error
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/storage/index_page_manager.cpp`:
- Around line 499-506: The page error must be published before waking waiters:
ensure the error is assigned (call new_page->SetError(err) or set the err_
state) prior to any code that notifies/wakes waiters; then wake waiters and
finally either FreeIndexPage(new_page) (if not pinned) or keep it pinned—i.e.,
move the SetError(err) (or err_ assignment) to occur before the
waiter-notification path so woken waiters observe the published error when
checking err_/NoError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0a545820-3826-4fa2-abe7-3239b70e7e49
📒 Files selected for processing (2)
include/storage/mem_index_page.hsrc/storage/index_page_manager.cpp
thweetkomputer
approved these changes
Mar 17, 2026
thweetkomputer
approved these changes
Mar 17, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/storage/index_page_manager.cpp`:
- Around line 498-506: The code leaves failed pages swizzled when IsPinned() is
true, which can cause detached callers to block on Wait() forever; modify the
logic so that before any detached handle calls Wait() it first checks
new_page->Error() and returns immediately if an error is set, OR alternatively
unswizzle/free the failed page regardless of pin status (move
mapping->Unswizzling(new_page) and FreeIndexPage(new_page) into the error branch
so SetError(err) is not the only action). Update the code paths around
new_page->IsPinned(), SetError(err), mapping->Unswizzling, FreeIndexPage, and
any caller that uses Wait() to consult Error() first to avoid indefinite
blocking.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 822cae4c-5f62-4222-a17f-5c7daa1c91d9
📒 Files selected for processing (1)
src/storage/index_page_manager.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/storage/index_page_manager.cpp (1)
498-507: Add regression coverage for the pinned failure path.This change now relies on
SetError(),WakeAll(), and the finalFreeIndexPage()after the last pin drops. That interaction is subtle, and the PR checklist still has tests unchecked.If helpful, I can sketch a loader+waiter
FindPage()test that exercises this branch.Also applies to: 518-526
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/storage/index_page_manager.cpp` around lines 498 - 507, Add a regression test exercising the pinned-failure path: create a loader that pins an index page via FindPage()/IsPinned(), simulate a failure that calls SetError(err) on the pinned page and invokes waiting_.WakeAll(), and concurrently start a waiter that calls FindPage() and waits on the page; assert the waiter observes the error, wakes, and that after the pin is released the page is freed (FreeIndexPage is invoked). Target the code paths around IsPinned, SetError, waiting_.WakeAll, and FreeIndexPage (also add the same test covering the similar branch around the lines referenced 518-526) to ensure the interaction between SetError/WakeAll and final FreeIndexPage after the last pin is exercised and prevents regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/storage/index_page_manager.cpp`:
- Around line 498-507: Add a regression test exercising the pinned-failure path:
create a loader that pins an index page via FindPage()/IsPinned(), simulate a
failure that calls SetError(err) on the pinned page and invokes
waiting_.WakeAll(), and concurrently start a waiter that calls FindPage() and
waits on the page; assert the waiter observes the error, wakes, and that after
the pin is released the page is freed (FreeIndexPage is invoked). Target the
code paths around IsPinned, SetError, waiting_.WakeAll, and FreeIndexPage (also
add the same test covering the similar branch around the lines referenced
518-526) to ensure the interaction between SetError/WakeAll and final
FreeIndexPage after the last pin is exercised and prevents regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 938e8394-9dfb-443a-93a6-f79a1ab39790
📒 Files selected for processing (1)
src/storage/index_page_manager.cpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Here are some reminders before you submit the pull request
fixes eloqdb/eloqstore#issue_idctest --test-dir build/tests/Summary by CodeRabbit